refactor code to make the API harder to misuse #261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sukunrt
reviewed
Jan 27, 2025
| @@ -70,135 +62,61 @@ func StringCast(s string) Multiaddr { | |||
| } | |||
|
|
|||
| // SplitFirst returns the first component and the rest of the multiaddr. | |||
| func SplitFirst(m Multiaddr) (*Component, Multiaddr) { | |||
There was a problem hiding this comment.
Maybe it's better to return (Multiaddr, Multiaddr)
The first multiaddr will be []Component{first}
With the current API, most users will need to do component.AsMultiaddr here.
This does break users who were expecting a component, but I believe that's the less popular usecase.
There was a problem hiding this comment.
I made this change and backed it out. I think a lot of usages was to get the component out, not a Multiaddr.
For example this is a pretty common pattern:
transport, p2ppart := ma.SplitLast(m)
if p2ppart == nil || p2ppart.Protocol().Code != ma.P_P2P {
return m, ""
}If we return a Multiaddr type, you can't use the Protocol() method like you were able to before.
This pattern mostly still works after this refactor, you just have to check p2ppart.Empty() instead of == nil
MarcoPolo
force-pushed
the
marco/multiaddr-refactor
branch
from
c652d4a to
f43a278
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This seeks to refactor the codebase to make it much harder to hit nil pointer dereference panics.
This takes a different approach to how we've treated multiaddrs in the past. Instead of attempting to make them a general and performant datastructure, we focus on treating them as just an encoding scheme. Users of multiaddrs are expected to parse the multiaddr into some struct that is suitable for their use case, and use the multiaddr form when interoperating. By treating Multiaddrs as just an encoding scheme we can make a number of simplifications in the codebase. Specifically this PR does the following:
Multiaddrinterface.[]ComponentComponentis now a public type, and users can useComponents directlyMultiaddrinterface as there is none.Background
This library has had multiple issues related to
Multiaddrbeing an interface. Many methods use and returnnilas the zero value, which behaves poorly when the user forgets to do a nil check on every returned value and attempts to call a method on the nil pointer. For example, usingSplitto split a Multiaddr and then usingJointo rebuild the original Multiaddr historically would panic in case one side of the split was nil. Using an interface also leads to incorrect usages of==to check if two Multiaddrs were equal (would only work for pointer equality) and incorrectly usingMultiaddras a key for a map.Using an interface is typically done to provide a consistent API surface for multiple implementing types. In practice however, the Multiaddr interface was only implemented for
multiaddrandcomponent(with arguably some awkwardness when using a component as a Multiaddr).The better approach is to use a concrete type for a Multiaddr. This lets pointer receiver methods work even if the pointer is nil, since the compiler already knows which function to call. Most methods now take a value rather than a pointer which avoids the issue of a nil pointer dereference completely.
Migration
Refer to ./v015-MIGRATION.md for breaking changes and migration tips